Skip to content

docs: correct upload create docs (add required fields)#439

Closed
Maksym Verbovyi (vermaxik) wants to merge 3 commits intomasterfrom
fix_openapi_upload
Closed

docs: correct upload create docs (add required fields)#439
Maksym Verbovyi (vermaxik) wants to merge 3 commits intomasterfrom
fix_openapi_upload

Conversation

@vermaxik
Copy link
Copy Markdown
Contributor

It's not possible to use Upload Create endpoint without those fields, they are required:

"file",
"file_format",
"locale_id"

@jablan
Copy link
Copy Markdown
Collaborator

jablan commented Nov 1, 2023

Maksym Verbovyi (@vermaxik) first, sorry for the delay, I was on vacation for a week.

The problem with this PR is that it's causing breaking change for all clients. Declaring an existing parameter as required changes method signatures. ruby example:

-    def upload_create(project_id, file, file_format, locale_id, opts = {})
-      data, _status_code, _headers = upload_create_with_http_info(project_id, file, file_format, locale_id, opts)
+    def upload_create(project_id, opts = {})
+      data, _status_code, _headers = upload_create_with_http_info(project_id, opts)
       data
     end

We'll have to discuss what to do in this case. IMO this should be merged, as it correctly describes the API, but it would need a major version bump for all the clients. Perhaps we should dedicate some time and investigate other potential places where this is needed, so that we have only one painful band-aid pull, rather than multiple ones in the future.

Sönke Behrendt (@theSoenke) what do you think?

@theSoenke
Copy link
Copy Markdown
Collaborator

jablan agree that we should spend some effort to check whether there are more such places to tackle all in one go. Although that might be some quite tedious work. Maybe we should then create a task for it?

@docstun
Copy link
Copy Markdown
Member

Maksym Verbovyi (@vermaxik) jablan what is the way forward here? Since tests are currently failing, I marked it as "draft" again.

@docstun Manuel Boy (docstun) marked this pull request as draft December 11, 2023 08:00
@jablan
Copy link
Copy Markdown
Collaborator

jablan commented Dec 11, 2023

Manuel Boy (@docstun) we opened a ticket for it https://phrase.atlassian.net/browse/TSI-2145 unfortunately, we can't make it during these quality weeks.

@theSoenke
Copy link
Copy Markdown
Collaborator

Maksym Verbovyi (@vermaxik) the logic for required fields is a bit more complex. file is definitely required but file_format and locale_id can be autodetected in some cases and are not required for all formats

@vermaxik
Copy link
Copy Markdown
Contributor Author

Hi Sönke Behrendt (@theSoenke)

thank you for checking it, good to know.

FYI there is possibility to add discriminator/polymorphism for dynamic request/response, TMS has it a lot.

Closing this PR in favor of #571

@jablan
Copy link
Copy Markdown
Collaborator

jablan commented Apr 22, 2024

FYI there is possibility to add discriminator/polymorphism for dynamic request/response, TMS has it a lot.

Maksym Verbovyi (@vermaxik) do they also generate a set of clients from OpenAPI description? I'm a bit concerned how different clients handle such cases.

@vermaxik
Copy link
Copy Markdown
Contributor Author

jablan looks like it should be supported https://openapi-generator.tech/docs/generators/openapi/#schema-support-feature

afaik TMS doesn't have any official clients (and they still on openapi v2), but at our team we need to implement custom support for this feature and would love to avoid it as much as we can.

@jablan jablan deleted the fix_openapi_upload branch December 19, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants